Redesign spirv-builder's watch API#422
Conversation
tuguzT
force-pushed
the
spirv-builder-watch-channel
branch
2 times, most recently
from
5037f91 to
66a5c6b
Compare
tuguzT
force-pushed
the
spirv-builder-watch-channel
branch
from
66a5c6b to
f241062
Compare
|
I thought CI issues were already resolved at GitHub side? |
tuguzT
requested review from
Firestar99,
LegNeato,
eddyb and
schell
as code owners
Firestar99
force-pushed
the
spirv-builder-watch-channel
branch
from
e5e56df to
f956b84
Compare
There was a problem hiding this comment.
I love the new design!
Just iterated some more on it, so I'll let others do the reviewing
My changes to it:
- we can just consume SpirvBuilder, if users need to clone it they can
- changing a file during recompilation no longer throws errors in stdout but will trigger exactly one recompile when you call
recv()again - wgpu example had a race condition between the first and watched compiles
Firestar99
force-pushed
the
spirv-builder-watch-channel
branch
from
4a9a64c to
aefe49e
Compare
|
I added a non-blocking |
Firestar99
force-pushed
the
spirv-builder-watch-channel
branch
from
aefe49e to
13a80b7
Compare
| return Err(SpirvWatcherError::WatchWithPrintMetadata.into()); | ||
| } | ||
|
|
||
| let (tx, rx) = sync_channel(1); |
There was a problem hiding this comment.
I've noticed that cargo-gpu produces duplicate last messages (finished compiling or failure) on branch Rust-GPU/cargo-gpu#117 on Windows.
Previously provided value was 0, now it is 1. Could this be the reason?
There was a problem hiding this comment.
Yes it is. When a file changes while a recompilation was running, your version would spit out an error into stderr due to the channel being full. This version uses () to record that something has happened and discards all further messages that fail to send due to the channel being full. Which allows edits during recompilation to correctly retrigger a followup recompilation. But it seems like most fs changes emit more than one event, so (almost*) all recompilations will trigger a second time. But I'd rather recompile again to figure out nothing has changed, than having a stale state due to dropping events during ongoing compilation.
(*it is a race condition between the notif thread submitting all events and the polling thread polling an event)
Firestar99
force-pushed
the
spirv-builder-watch-channel
branch
from
13a80b7 to
1bbd238
Compare
schell
left a comment
schell
left a comment
There was a problem hiding this comment.
Great work! I just had a nitpick comment about the docs.
Firestar99
force-pushed
the
spirv-builder-watch-channel
branch
from
1bbd238 to
6aa1dc9
Compare
3 participants
Required by Rust-GPU/cargo-gpu#117
It was suggested to rewrite API from scratch instead of patching the existing one.
My idea now is to expose refactored
Watcher, which could just returnCompileResultonrecv(), then we don't need to accept any callbacks, users don't need to differentiate first compile result from others, etc.